Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix redirection #694

Closed
wants to merge 5 commits into from
Closed

Fix redirection #694

wants to merge 5 commits into from

Conversation

erikzhang
Copy link
Member

Fix #693

Ashuaidehao and others added 4 commits December 15, 2021 10:27
* init

* upgrade neo

* useDiagnostic

* Update RpcServer.SmartContract.cs

* invoke tree

* add event

* Add storage changes

* Update nuget

* rename event state

* Unify

* update neofs

* fix json

Co-authored-by: Shargon <[email protected]>
Co-authored-by: Erik Zhang <[email protected]>
Co-authored-by: Owen Zhang <[email protected]>
* add in file copyright

* fix the copyright

* update copyright start year

* Delete copyright.sh

* Delete copyright.txt

Co-authored-by: Owen Zhang <[email protected]>
Co-authored-by: Erik Zhang <[email protected]>
* init

* refac

Co-authored-by: Shargon <[email protected]>
@erikzhang erikzhang requested a review from shargon March 2, 2022 03:34
@erikzhang erikzhang mentioned this pull request Mar 2, 2022
@@ -62,6 +72,14 @@ public async Task<(OracleResponseCode, string)> ProcessAsync(Uri uri, Cancellati
return (OracleResponseCode.Error, null);
if (!Settings.Default.AllowedContentTypes.Contains(message.Content.Headers.ContentType.MediaType))
return (OracleResponseCode.ContentTypeNotSupported, null);

if (!Settings.Default.AllowPrivateHost && message.RequestMessage.RequestUri != uri)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it really works to prevent ssrf since the request is already sent in the line:

message = await client.GetAsync(uri, HttpCompletionOption.ResponseContentRead, cancellation);

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. It will leak information such as resource existence at least because this check is not the first branch.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with @vang1ong7ang , I think that the redirection must be manual like #692

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, should not assume GET requests have no side effect.

@vang1ong7ang
Copy link

i prefer to avoid the request happening. not only the final content uploading.

@vang1ong7ang
Copy link

dns rebinding should be also considered

@dusmart
Copy link

dusmart commented Mar 2, 2022

dns rebinding should be also considered

It seems that it's difficult to prevent SSRF in .Net.
Only a doc for Python is found here in Chinese.

@vang1ong7ang
Copy link

@dusmart also in golang i think

@erikzhang
Copy link
Member Author

Since we support https only, I think we don't need to worry about dns rebinding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

oracle service vulnerability: local information leak
6 participants